-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate CHOMP and collision tutorials from ROS1 #492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
I put in quite a few nitpicks on the original tutorial - I realize some of these aren't related to your changes, but I think it's a good opportunity to bit of cleanup.
doc/examples/collision_environments/src/collision_scene_example.cpp
Outdated
Show resolved
Hide resolved
doc/examples/collision_environments/src/collision_scene_example.cpp
Outdated
Show resolved
Hide resolved
doc/examples/collision_environments/src/collision_scene_example.cpp
Outdated
Show resolved
Hide resolved
doc/examples/collision_environments/src/collision_scene_example.cpp
Outdated
Show resolved
Hide resolved
doc/examples/collision_environments/src/collision_scene_example.cpp
Outdated
Show resolved
Hide resolved
doc/examples/collision_environments/src/collision_scene_example.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested out the functionality now, it looks good 👍
I like the way you mixed the collision object tutorial in with the CHOMP tutorial.
Will approve after you address the remaining minor comments.
rviz_full_config = os.path.join(rviz_base, "moveit_chomp.rviz") | ||
rviz_empty_config = os.path.join(rviz_base, "moveit_chomp.rviz") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these lines are meant to be the same, do we need to have two configuration variables and the IfCondition for rviz_node_tutorial
and rviz_node
?
This pull request is in conflict. Could you fix it @MikeWrock? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good pending the question about rviz_full_config
and rviz_empty_config
looking like the same thing.
This pull request is in conflict. Could you fix it @MikeWrock? |
1 similar comment
This pull request is in conflict. Could you fix it @MikeWrock? |
* Migrated tutorial from ROS1 * Fix merging error in CMakeLists.txt --------- Co-authored-by: Henning Kayser <[email protected]> Co-authored-by: Sebastian Jahr <[email protected]> (cherry picked from commit 725c7d8) # Conflicts: # CMakeLists.txt # doc/examples/chomp_planner/chomp_planner_tutorial.rst # doc/examples/planning_adapters/planning_adapters_tutorial.rst # doc/how_to_guides/how_to_guides.rst
… humble (#742) * Migrate CHOMP and collision tutorials from ROS1 (#492) * Migrated tutorial from ROS1 * Fix merging error in CMakeLists.txt --------- Co-authored-by: Henning Kayser <[email protected]> Co-authored-by: Sebastian Jahr <[email protected]> (cherry picked from commit 725c7d8) # Conflicts: # CMakeLists.txt # doc/examples/chomp_planner/chomp_planner_tutorial.rst # doc/examples/planning_adapters/planning_adapters_tutorial.rst # doc/how_to_guides/how_to_guides.rst * Address rebasing conflicts * Remove references to non-existing files and cleanup * Cleanups * Update doc/examples/examples.rst --------- Co-authored-by: Michael Wrock <[email protected]> Co-authored-by: Sebastian Jahr <[email protected]>
Description
Please explain the changes you made, including a reference to the related issue if applicable
Checklist